Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[base-ui][NumberInput] Remove inputId and inputRef types from NumberInput component #40425

Merged
merged 12 commits into from
Jan 10, 2024

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Jan 4, 2024

inputId is not a valid prop for NumberInput component (check here) but NumberInput component type was allowing it. This PR removes prop from type

@sai6855 sai6855 added bug 🐛 Something doesn't work typescript package: base-ui Specific to @mui/base component: number field This is the name of the generic UI component, not the React module! labels Jan 4, 2024
@mui-bot
Copy link

mui-bot commented Jan 4, 2024

Netlify deploy preview

https://deploy-preview-40425--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 8ea34d2

@sai6855 sai6855 marked this pull request as draft January 4, 2024 07:29
@sai6855 sai6855 marked this pull request as ready for review January 4, 2024 08:26
@sai6855 sai6855 changed the title [base-ui][NumberINput] Remove inputId type from NumberInput types [base-ui][NumberInput] Remove inputId type from NumberInput types Jan 4, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even the inputRef isn't documented for NumberInput. Do you think it should be available on NumberInput? I believe it should.

@sai6855
Copy link
Contributor Author

sai6855 commented Jan 10, 2024

Even the inputRef isn't documented for NumberInput. Do you think it should be available on NumberInput? I believe it should.

I don't think inputRef is a valid prop. if you check below sandbox line 16,17, only line 17 logs input element as it is passed through slotProps.input where as line 16 doesn't. as it was passed through inputRef.

https://codesandbox.io/p/sandbox/funny-knuth-kc8h4d?file=%2Fsrc%2FDemo.tsx%3A17%2C17-17%2C29

So i think inputRef should be removed from NumberInput Type. Removed in 8ea34d2

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Jan 10, 2024

Even the inputRef isn't documented for NumberInput. Do you think it should be available on NumberInput? I believe it should.

I don't think inputRef is a valid prop. if you check below sandbox line 16,17, only line 17 logs input element as it is passed through slotProps.input where as line 16 doesn't. as it was passed through inputRef.

https://codesandbox.io/p/sandbox/funny-knuth-kc8h4d?file=%2Fsrc%2FDemo.tsx%3A17%2C17-17%2C29

So i think inputRef should be removed from NumberInput Type. Removed in 8ea34d2

It's currently not available, but shouldn't we support it by adding the implementation? Although it's supported in slotProps.input, we can make it a direct prop on NumberInput and pass it to the useNumberInput hook. Currently, it's only available on useNumberInput.

As for inputId, the id prop of NumberInput was considered as inputId, so it's correct that it shouldn't be available for NumberInput. However, for user clarity, it might be better to explicitly use inputId in NumberInput for input and id for root.

In essence, all the props for useNumberInput should be available on NumberInput since the hook is used to build the NumberInput component internally.

If you agree, these changes can be done in a separate PR. If implemented in a separate PR, we would need to revert 8ea34d2 here. Alternatively, you can include all these changes in this PR, altering its purpose altogether.

@sai6855
Copy link
Contributor Author

sai6855 commented Jan 10, 2024

I agree with you on providing inputId and id, but if you look at https://mui.com/base-ui/react-input/components-api/. Even Input component doesn't have explicit inputId and inputRef ( even though hook version of input accepts inputRef) . So maybe it was conscious choice for not allowing inputRef, inputId on Input and NumberInput components.

In essence, all the props for useNumberInput should be available on NumberInput since the hook is used to build the NumberInput component internally.

I'm not sure if above hypothesis is true, because if check https://mui.com/base-ui/react-button/hooks-api/ and https://mui.com/base-ui/react-button/components-api/ props, there are multiple props which are not common in component version and hook version. Also Difference in props across hook and component versions is not just limited to Button component, many components have it

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Jan 10, 2024

I agree with you on providing inputId and id, but if you look at https://mui.com/base-ui/react-input/components-api/. Even Input component doesn't have explicit inputId and inputRef ( even though hook version of input accepts inputRef) . So maybe it was conscious choice for not allowing inputRef, inputId on Input and NumberInput components.

In essence, all the props for useNumberInput should be available on NumberInput since the hook is used to build the NumberInput component internally.

I'm not sure if above hypothesis is true, because if check https://mui.com/base-ui/react-button/hooks-api/ and https://mui.com/base-ui/react-button/components-api/ props, there are multiple props which are not common in component version and hook version. Also Difference in props across hook and component versions is not just limited to Button component, many components have it

Alright, I see. Then this PR looks good.

@ZeeshanTamboli ZeeshanTamboli changed the title [base-ui][NumberInput] Remove inputId type from NumberInput types [base-ui][NumberInput] Remove inputId and inputRef types from NumberInput component Jan 10, 2024
@ZeeshanTamboli ZeeshanTamboli merged commit 05f3093 into mui:master Jan 10, 2024
19 checks passed
@ZeeshanTamboli ZeeshanTamboli mentioned this pull request Jan 10, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: number field This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants